Skip to content

server: save processed mtmd chunks as placeholder - #27278

Merged
ngxson merged 1 commit into
masterfrom
xsn/server_use_mtmd_placeholder
Aug 17, 2026
Merged

server: save processed mtmd chunks as placeholder#27278
ngxson merged 1 commit into
masterfrom
xsn/server_use_mtmd_placeholder

Conversation

@ngxson

@ngxson ngxson commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Overview

Save memory by only store mtmd chunks as placeholder in server_prompt object

Placeholder chunks can be used for counting tokens and positions, but doesn't contain underlay data buffer

Requirements

@ngxson
ngxson requested review from a team as code owners August 17, 2026 17:44
@github-actions github-actions Bot added server mtmd Related to multimodal functionality (video/image/audio) labels Aug 17, 2026
@ngxson

ngxson commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

/bot review

@ggml-gh-bot

ggml-gh-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown
Automated code review

I reviewed the diff (mtmd get_placeholder helper + server_tokens::push_back_placeholder + the single call site in server-context.cpp). I traced how slot.prompt.tokens media chunks are consumed afterwards (cache-hit comparison via find_diff, validate, find_message_spans, slot save/restore via serialize/deserialize, slot clone()) and confirmed placeholders preserve the only fields those paths read (type, id, n_tokens, n_pos), since serialize already strips only the batch_f32 buf and keeps all metadata. Context shift is already aborted for multimodal, so the stripped chunks are never re-encoded. The change is semantically sound.

Will slow the review

(point 1) New public API in mtmd.h (mtmd_input_chunk_get_placeholder) with a single internal consumer. The only caller is server_tokens::push_back_placeholder in tools/server/server-common.cpp:514. Per the multimodal guidance, new public surface in mtmd.h is expected to be discussed first, and here it isn't obviously needed as public ABI: it exists purely to let server-common strip chunk data. Consider implementing this as an internal helper (e.g. a method on the internal mtmd_input_chunk struct, or a non-exported function in mtmd.cpp that server-common reaches through a thin existing accessor) rather than widening the public C API for one in-tree user. If it must stay public, add a one-line justification in the PR description.

Nits

(point 2) tools/server/server-common.cpp:516 aborts the whole server on placeholder failure:

GGML_ASSERT(new_chunk != nullptr && "failed to create placeholder chunk");

The previous push_back path went through mtmd_input_chunk_copy, which can only fail by throwing bad_alloc (not by aborting). mtmd_input_chunk_get_placeholder can return nullptr if the serialize/roundtrip throws (logged via LOG_ERR). A roundtrip of an internally-valid chunk shouldn't fail, but if it ever does, GGML_ASSERT takes down the server process where the old code would not. Prefer returning the error to the caller of push_back_placeholder (and ultimately send_error/slot.release) instead of asserting.

(point 3) tools/mtmd/mtmd.cpp:2338 implements "strip data" as a serialize -> deserialize roundtrip through a temporary std::vector<char>, allocated and re-parsed for every media chunk during prompt processing. The author's own comment calls it "hacky". A direct metadata-only clone (copy nx/ny/pos/image_idx/n_temporal_merge/id and batch_f32 entries with cleared buf) would avoid the extra allocation + re-parse and read more clearly. Not blocking since it is correct and the serialized form is small.

(point 4) tools/mtmd/mtmd.cpp:2316mtmd_input_chunk_save_impl's LOG_ERR uses __func__, which now prints mtmd_input_chunk_save_impl instead of the public mtmd_input_chunk_save. Minor loss of debuggability; consider forwarding the caller name or keeping the log at the public boundary.

(point 5) Comment wording: tools/mtmd/mtmd.cpp:2339 "faster than copy the whole batch data" -> "faster than copying"; tools/server/server-common.h:197 and tools/mtmd/mtmd.h description "doesn't contain underlay data buffer" -> "underlying".

This review was generated automatically by pi coding agent using zai-org/GLM-5.2. It may contain mistakes. Maintainers make the final call.

@ngxson
ngxson merged commit 533b182 into master Aug 17, 2026
26 of 27 checks passed
gabe-l-hart added a commit to gabe-l-hart/llama.cpp that referenced this pull request Aug 17, 2026
* origin/master: (86 commits)
ui: enforce alphabetical enum member ordering (ggml-org#27272)
ui: Refactor Built-In Tools naming (Server/Browser) (ggml-org#27271)
ci: more optimizations (ggml-org#26983)
doc: document MCP stdio servers and CORS defaults in the server README [no release] [no ci] (ggml-org#26847)
server: save processed mtmd chunks as placeholder (ggml-org#27278)
mtmd: use sha256 for input hashing (ggml-org#27274)
vocab : support integer tokenizer scores (ggml-org#27260)
mtmd : skip thumbnail for non-tiled LFM2 images (ggml-org#27246)
cuda : skip UMA override for HIP builds (ggml-org#27083)
vendor: move hash to vendor (ggml-org#27262)
ci : push release tag explicitly in release.yml (ggml-org#27261)
ui: move get_datetime tool to frontend (ggml-org#27255)
ci : reduce builds in build-xcframework.sh (ggml-org#27252)
model: support speculators-format checkpoints for DSpark (ggml-org#26275)
ui: add browser get_info tool (ggml-org#27251)
ci : restore release.yml check during make-release.yml (ggml-org#27247)
mtmd: harden preprocessor_granite (ggml-org#27235)
ci : allow make-release to target a specific commit (ggml-org#27234)
ci : make release workflows use a deploy key (ggml-org#27229)
convert: add @ModelBase.example (ggml-org#27208)
...
brittlewis12 pushed a commit to brittlewis12/llama.cpp that referenced this pull request Aug 17, 2026
Madreag pushed a commit to Madreag/turbo3-cuda that referenced this pull request Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mtmd Related to multimodal functionality (video/image/audio) server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant